Windows: Emulate MSG_PEEK by read #19777

soulxu · 2022-02-02T00:03:31Z

Implementing peek by draining the socket and storing the
content of the socket to a buffer.

Additional Description: See #17395
Risk Level: Medium
Testing: Automated
Docs Changes: Pending
Release Notes: Pending
Platform-Specific Features: Windows only

Signed-off-by: He Jie Xu hejie.xu@intel.com
Co-authored-by: Sotiris Nanopoulos sonanopo@microsoft.com

Commit Message: The way we implement i/o events on windows poses problems with peeking messages. The reason for that is that peeking does not drain the buffer which is an implicit requirement. This can cause slow clients to make envoy connections to hang. We solve this issue by implementing peek by draining the socket and storing the content of the socket to a buffer. Additional Description: See envoyproxy#17395 Risk Level: Medium Testing: Automated Docs Changes: Pending Release Notes: Pending Platform Specific Features: Windows only Co-authored-by: He Jie Xu <hejie.xu@intel.com> Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-02-02T00:06:09Z

/cc @adisuissa @alyssawilk

thanks @adisuissa give me the error info, and apologize for not making the code right.

@adisuissa said there still other errors, appreciate if can help me spot them out, then let me fix it.

soulxu · 2022-02-02T00:07:05Z

source/common/buffer/buffer_impl.cc

+    // The length to copy should be size of smallest in the source slice available size and
+    // the dest slice available size.
+    uint64_t length_to_copy =
+        std::min(left_data_size_in_src_slice, std::min(left_data_size_in_dst_slice, left_to_read));


I correct the length_to_copy calculation.

soulxu · 2022-02-02T00:07:26Z

source/common/buffer/buffer_impl.cc

+      dest_slice_offset = 0;
+    }
+    ASSERT(src_slice_offset <= src_slice.dataSize());
+    ASSERT(dest_slice_offset <= dest_slice.len_);


Add two assertions for defending.

adisuissa · 2022-02-02T02:17:27Z

@adisuissa said there still other errors, appreciate if can help me spot them out, then let me fix it.

There is a fuzz error that seems to be in the fuzz test itself, and not due to this PR.
The error is being investigated separately.

soulxu · 2022-02-02T03:19:49Z

@adisuissa said there still other errors, appreciate if can help me spot them out, then let me fix it.

There is a fuzz error that seems to be in the fuzz test itself, and not due to this PR. The error is being investigated separately.

got it, thanks

soulxu · 2022-02-02T05:48:27Z

/retest

repokitteh-read-only · 2022-02-02T05:48:31Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19777 (comment) was created by @soulxu.

see: more, trace.

soulxu · 2022-02-10T23:54:25Z

@alyssawilk would you mind take a look at this pr again? Thanks in advance!

alyssawilk · 2022-02-15T14:55:24Z

tagging Adi for first pass since @wrowe has not been responding.

soulxu · 2022-02-15T15:49:47Z

tagging Adi for first pass since @wrowe has not been responding.

Thanks!

wrowe · 2022-02-15T16:16:56Z

@davinci26 as much of this was your code, and we still have you on the assignable, envoy-triage and windows-dev teams, are you willing to take a pass at this? [If you would like us to modify your teams we can do that, too.]

davinci26 · 2022-02-15T16:17:50Z

I will review later today

davinci26

overall lgtm, thanks for all the hard work on this

test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu · 2022-02-16T03:19:02Z

overall lgtm, thanks for all the hard work on this

thanks! done the clean up for those useless logs in the tests.

soulxu · 2022-02-16T07:29:20Z

/retest

repokitteh-read-only · 2022-02-16T07:29:24Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19777 (comment) was created by @soulxu.

see: more, trace.

wrowe

LGTM, thanks for all your work on this!

Sotiris Nanopoulos and others added 30 commits November 4, 2021 09:24

fix format

c2b5b44

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix compiler warning

fd85009

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix compilation issue

8972610

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

more coverage is needed but still testing ci

4aeaeae

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Merge remote-tracking branch 'upstream/main' into peekWindows2

544a1c8

fix format and increase coverage

15fc820

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix format

4e60815

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix gcc

fe11b35

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

address PR feedack v1

1534dcd

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

address comments

3eb8880

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix spelling

f2fd1fb

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix format

72e8f20

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

add override keyword

db35fbf

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

another format fix

16caa4f

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix string buffer used in fuzzing

a479ef8

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fix typo in fuzzer

d3dd4d3

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

add override keyword

23c6341

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

fixed another typo

1c1223f

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

remove unused parameters

abe8be2

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

PR comments and tests

31aee37

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

add hint for mac compiler

55361a3

Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

Improve the copyOutToSlices method

320e370

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Merge branch 'main' into peekWindows2

f73213d

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

address comment

a2e8799

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

type conversion for macos

d113f4c

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

correct the copyOutToSlices method

0c03750

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Merge branch 'main' into peekWindows2

52828ee

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

remove NOT_IMPLEMENTED_GCOVR_EXCL_LINE

959170b

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Merge branch 'main' into peekWindows2

d023827

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu added 3 commits January 24, 2022 16:03

Merge branch 'main' into peekWindows2

843ca3c

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

Merge branch 'main' into peekWindows3

4bf0aff

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

fix copyOutToSlices

cf83aad

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu requested a review from dio as a code owner February 2, 2022 00:03

soulxu commented Feb 2, 2022

View reviewed changes

snowp assigned wrowe Feb 4, 2022

soulxu mentioned this pull request Feb 10, 2022

event: assert the case of both read and closed event registered #18265

Merged

wrowe requested a review from davinci26 February 10, 2022 17:29

soulxu mentioned this pull request Feb 11, 2022

listener: remove the peek from the listener filters #17395

Merged

alyssawilk assigned adisuissa and alyssawilk Feb 15, 2022

davinci26 previously approved these changes Feb 16, 2022

View reviewed changes

test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc Outdated Show resolved Hide resolved

test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc Outdated Show resolved Hide resolved

remove useless log

d789aae

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu dismissed davinci26’s stale review via d789aae February 16, 2022 03:18

wrowe approved these changes Feb 16, 2022

View reviewed changes

wrowe merged commit 7cddecb into envoyproxy:main Feb 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows: Emulate MSG_PEEK by read #19777

Windows: Emulate MSG_PEEK by read #19777

soulxu commented Feb 2, 2022

soulxu commented Feb 2, 2022

soulxu Feb 2, 2022

soulxu Feb 2, 2022

adisuissa commented Feb 2, 2022

soulxu commented Feb 2, 2022

soulxu commented Feb 2, 2022

repokitteh-read-only bot commented Feb 2, 2022

soulxu commented Feb 10, 2022 •

edited

Loading

alyssawilk commented Feb 15, 2022

soulxu commented Feb 15, 2022

wrowe commented Feb 15, 2022

davinci26 commented Feb 15, 2022

davinci26 left a comment

soulxu commented Feb 16, 2022

soulxu commented Feb 16, 2022

repokitteh-read-only bot commented Feb 16, 2022

wrowe left a comment

Windows: Emulate MSG_PEEK by read #19777

Windows: Emulate MSG_PEEK by read #19777

Conversation

soulxu commented Feb 2, 2022

soulxu commented Feb 2, 2022

soulxu Feb 2, 2022

Choose a reason for hiding this comment

soulxu Feb 2, 2022

Choose a reason for hiding this comment

adisuissa commented Feb 2, 2022

soulxu commented Feb 2, 2022

soulxu commented Feb 2, 2022

repokitteh-read-only bot commented Feb 2, 2022

soulxu commented Feb 10, 2022 • edited Loading

alyssawilk commented Feb 15, 2022

soulxu commented Feb 15, 2022

wrowe commented Feb 15, 2022

davinci26 commented Feb 15, 2022

davinci26 left a comment

Choose a reason for hiding this comment

soulxu commented Feb 16, 2022

soulxu commented Feb 16, 2022

repokitteh-read-only bot commented Feb 16, 2022

wrowe left a comment

Choose a reason for hiding this comment

soulxu commented Feb 10, 2022 •

edited

Loading